Conversation
|
I'm a total noob to the PROJ C API, but from reading https://proj.org/en/9.3/development/reference/functions.html#transformation-setup it seems that a If that diagnosis is accurate, would the best course of action be to create a new |
|
Looking at the API, it looks like it returns a PJ object. I would be surprised if they were somehow different, but I'm not at an actual screen right now. I'll try to have a look tomorrow though. |
|
We are not even making it as far as the call to |
|
When I get time to come back to this, I'll try to look at how pyproj is using the proj api |
|
It looks like the options aren't being constructed correctly: if I do let opts_pp = CString::new("").unwrap();
let opts_pp = opts_pp.as_ptr();
…
let out_ptr = proj_as_projjson(self.ctx, self.c_proj, opts_pp as _);I get a JSON string back. So this should be an easy fix. |
|
Whoops 😅
I'm not exactly sure where I went wrong looking at the code |
|
Actually perhaps I spoke too soon: this doesn't work, though it's possible I've messed up the pointer logic to go from That gives me: And then panics because a null pointer is returned which triggers our |
|
I had been trying to use this as a model: Lines 322 to 329 in 8db09d6 |
|
It turns out that two things were wrong here: one is a bit more obvious and we should have caught it. The other thing is not obvious and caused this to take a couple of hours to debug because every time I changed the test from "pass some options" to "pass no options" it would start failing again.
I pushed my working changes to your branch – feel free to adapt! |
|
Sorry to make you take a couple hours of debugging, but awesome to see it working! With this learning, is there anything that should be reflected in the It looks like the CI is failing on a dependency that was bumped above proj's msrv |
|
Oh it's nobody's fault. I think the reason it hasn't been caught is that I had no expectation that someone would try to pass an empty string to On the failing CI, I think we'll have to switch to the newest georust CI images in proj. |
|
|
src/proj.rs
Outdated
| // it seems wasteful to leave the None check until now, but building the option pointers inside | ||
| // an if statement consistently causes libproj to segfault with a memory error due to bad input |
There was a problem hiding this comment.
I thought... that sounds weird! Surely it must work in an if statement. And I pushed a commit and found that did not work 🙈
There was a problem hiding this comment.
That is so weird. Feel free to unwind/write over the last commit
|
Well, I was testing a little more with the latest commit I pushed and I'm getting which looks like it's coming from here: So it would be nice to understand what about these cstrings is going wrong |
src/proj.rs
Outdated
| // > used inside the unsafe block: | ||
| let out_ptr = if let Some(opts_p) = opts_p { | ||
| let opts_c = opts_p.iter().map(|x| x.as_ptr()).collect::<Vec<_>>(); | ||
| proj_as_projjson(self.ctx, self.c_proj, opts_c.as_ptr()) |
There was a problem hiding this comment.
This probably needs to finish with a null.
There was a problem hiding this comment.
Sorry what does finish with a null mean?
There was a problem hiding this comment.
Push (or chain()) a ptr::null() at the end, otherwise it's unterminated.
There was a problem hiding this comment.
I'm confused too. At the end of what?
|
@urschrei I think I'm just learning that the way you had it was correct, but I had to make changes to learn that 😅 |
|
No, the previous version was fine except for the missing null at the end. |
|
The options are an array of C-style strings, not a single string. But the array has no length, so it had to end with a null pointer. You can see the implementation at https://github.com/OSGeo/PROJ/blob/master/src/iso19111/c_api.cpp#L1812. |
|
https://github.com/georust/gdal/blob/master/src/dataset.rs#L108 follows the same pattern. |
Ohhhh OK. Now I get it. |
a9e6231 to
126a8ec
Compare
126a8ec to
4248536
Compare
|
@kylebarron I've added a new error (since I think this is ready for review / merge now. |
|
Awesome, sounds good! |
|
Would love to get this merged! Thanks @urschrei for your help! |
This is a first attempt at solving #183 but the doctest segfaults 😕 . Does anyone have guidance here?